Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metric for message processing latency #1956

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

eleftherias
Copy link
Contributor

@eleftherias eleftherias commented Dec 18, 2023

Fix #1839

Opted for a histogram instead of gauge, because the library doesn't have a synchronous gauge.

@eleftherias eleftherias requested a review from a team as a code owner December 18, 2023 17:16
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this Middleware is only added to the executor. If you want to capture general message processing, you should add this to the eventer itself

evankanderson
evankanderson previously approved these changes Dec 19, 2023
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Histogram may be more useful in the future, if we end up with an implementation which can handle multiple in-flight events at once (i.e. out-of-order execution). I almost thought about asking for one, then decided it was overkill. 😁

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss where this shows go first. I think it should be in the eventer instead.

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, meant to request changes.

@evankanderson
Copy link
Member

I'd be comfortable with a Middleware. I'm also wondering whether we should contribute to https://github.com/ThreeDotsLabs/watermill/blob/master/components/metrics/handler.go#L62 rather than adding our own additional Middleware.

@evankanderson
Copy link
Member

Actually, we might want to hook DecoratePublisher and DecorateSubscriber from here: https://github.com/ThreeDotsLabs/watermill/blob/v1.3.5/components/metrics/builder.go to set the Metadata fields we need.

@evankanderson
Copy link
Member

I filed ThreeDotsLabs/watermill#417 to see if this would be accepted upstream. In the meantime, it feels like it would make sense to implement this as a Middleware / PublisherDecorator in eventer.go.

@eleftherias eleftherias force-pushed the 1839-processing-time branch 2 times, most recently from fd1c2bd to 6506d63 Compare December 21, 2023 15:33
@eleftherias
Copy link
Contributor Author

@JAORMX @evankanderson I moved the code into the eventer.
I see a few options for where the metric can be read, let me know if you think there's a better place for it than the handler function.

internal/events/eventer.go Outdated Show resolved Hide resolved
internal/events/eventer.go Outdated Show resolved Hide resolved
internal/events/eventer.go Outdated Show resolved Hide resolved
internal/events/eventer.go Outdated Show resolved Hide resolved
@JAORMX JAORMX self-requested a review December 22, 2023 12:01
Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shipit 🚢

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, the unit test issue seems legit

@eleftherias
Copy link
Contributor Author

oops, the unit test issue seems legit

@JAORMX On it.

@eleftherias eleftherias merged commit a966cd3 into mindersec:main Dec 22, 2023
16 checks passed
@eleftherias eleftherias deleted the 1839-processing-time branch December 22, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a metric to report on message publish -> process latency
3 participants